SPT: Isolate TemplateSelectorControl component #38445

ockham · 2019-12-16T08:09:07Z

Changes proposed in this Pull Request

Isolate TemplateSelectorControl component, and rename to TemplateSelector.

While working on #38403, I discovered that TemplateSelectorControl's styling is a bit weird, and doesn't lend itself easily to removing the white bar at the bottom of each item that contains the template name.

This is due to some padding-top: 100%, absolute positioning, translating, etc. I think that we can achieve the same styling in a more semantic way, that will also allow us to conditionally remove the label.

To that end, I'd like to isolate the TemplateSelectorControl component and styling, so it becomes easier to modify them, since currently, there's just one big style file in FSE.

This PR renames TemplateSelectorControl to TemplateSelector, and moves it (and its dependencies, and styling) to a separate folder.

Testing instructions

Verify that Gutenboarding still looks the same as before.
Build FSE and test it in your WP.com sandbox, and verify that it also still looks and feels the same.
Run tests in apps/full-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/components/template-selector/test/:
- cd apps/full-site-editing/ && npm ci && npm run test:unit

matticbot · 2019-12-16T08:09:09Z

Test live: https://calypso.live/?branch=isolate/spt-template-selector

sirreal

I left a few questions about SASS variables.

sirreal · 2019-12-16T09:04:09Z

...-plugin/starter-page-templates/page-template-modal/components/template-selector/preview.scss

+// Preview positioning
+$preview-max-width: 960px;
+$preview-right-margin: 24px;
+$preview-left-sidebar-reduced: calc( 30% + #{$wp-org-sidebar-reduced} );
+$preview-left-sidebar-full: calc( 30% + #{$wp-org-sidebar-full} );
+
+$template-large-preview-title-height: 120px;


I believe these will become global, there's no module-scoping of variables in SASS.

It would be good to make sure "local" variables have namespace style prefixes.

I copied all of this from existing SPT styling 😅 Think there's potential for regression?

sirreal · 2019-12-16T09:05:12Z

...g-plugin/starter-page-templates/page-template-modal/components/template-selector/shared.scss

+$wp-org-sidebar-reduced: 36px;
+$wp-org-sidebar-full: 160px;


Are these meant to replace some existing variables? This seems dangerous, see my other comment about global variables and namespaces.

I think they're just used by SPT for some calculations.

We could use these upstream vars though: https://github.com/WordPress/gutenberg/blob/fdd69535ecbec018bfabad783c890de3d72b7e07/packages/base-styles/_variables.scss#L30-L32

Pushed f4adda3 and ba960df to that effect.

simison · 2019-12-16T10:48:35Z

...g-plugin/starter-page-templates/page-template-modal/components/template-selector/shared.scss

+$template-selector-modal-offset-right: 32px;
+$template-selector-blank-template-mobile-height: 70px;
+
+@mixin screen-reader-text() {


Isn't this mixin either in Calypso or Gutenberg base styles already? Could we import it from there?

I couldn't find it in Calypso upon a quick search. It appears to be in Gutenberg, https://github.com/WordPress/gutenberg/blob/bfd1117846c7802d62dbba046c27968d961781e7/packages/block-editor/src/components/responsive-block-control/style.scss#L1-L12, so we might be able to use that -- unless there's more implicit dependencies (like this: WordPress/gutenberg#19159).

Overall, this PR is just moving around pre-existing styling; I'd like to limit changes that go beyond that in order to unblock it soon 😬

(We might actually want to use that entire component rather than just its styling.)

Just moving things around in this PR makes sense. 👍

simison · 2019-12-16T10:49:38Z

...g-plugin/starter-page-templates/page-template-modal/components/template-selector/shared.scss

+$template-selector-border-color-selected: #555d66;
+$template-selector-border-color-active: #00a0d2;
+$template-selector-border-color-hover: #c9c9ca;
+$template-selector-empty-background: #fff;


While it's ok to have them here, should these variables be pulled in from Gutenberg base styles or color studio? I.e.:

$template-selector-empty-background: $white;

simison · 2019-12-16T10:50:17Z

...-plugin/starter-page-templates/page-template-modal/components/template-selector/preview.scss

+
+// Template Selector Preview
+.template-selector-preview {
+	@media screen and ( max-width: 659px ) {


Do we have mixins or variables available in Gutenberg for these screen sizes?

Couldn't find any (tried grepping 660px and 659px).

Gotcha — I guess in some follow up some day we could re-use breakpoints from ~@wordpress/base-styles/breakpoints

simison · 2019-12-16T10:51:08Z

...-plugin/starter-page-templates/page-template-modal/components/template-selector/preview.scss

+$preview-max-width: 960px;
+$preview-right-margin: 24px;
+$preview-left-sidebar-reduced: calc( 30% + #{$admin-sidebar-width-collapsed} );
+$preview-left-sidebar-full: calc( 30% + #{$admin-sidebar-width} );


Will $admin-sidebar-width work both in wp-admin when WP sidebar is visible and when Gutenberg is iframed in Calypso and the sidebar is hidden?

idk 😅 -- But all I did in f4adda3 is remove two SASS var definitions with two other ones that I'm importing from @wordpress/base-styles. That shouldn't change any behavior, should it?

That shouldn't change any behavior, should it?

It shouldn't indeed and sounds fair! :-) Just good to test both scenarios anyway always when CSS is touched for anything FSE plugin related.

simison · 2019-12-16T10:52:55Z

Left a few scss variables questions as well — I haven't tested so I'm little ignorant about how things actually work, hence some questions might not make sense. 😁 Hope this helps anyway!

simison · 2019-12-16T11:07:37Z

...-plugin/starter-page-templates/page-template-modal/components/template-selector/preview.scss

@@ -0,0 +1,189 @@
+@import '~@wordpress/base-styles/variables';


Since WordPress/gutenberg#19159 is an issue at the moment, you could also import the whole set via this file:

wp-calypso/client/assets/stylesheets/gutenberg-base-styles.scss

Lines 1 to 6 in f2b1724

@import '~@wordpress/base-styles/z-index';

@import '~@wordpress/base-styles/colors';

@import '~@wordpress/base-styles/variables';

@import '~@wordpress/base-styles/breakpoints';

@import '~@wordpress/base-styles/mixins';

@import '~@wordpress/base-styles/animations';

I pushed be80333 which seems to be enough.

Copons · 2019-12-16T11:27:33Z

Ehm... this PR says hi: #38065 😅

ockham · 2019-12-17T09:53:37Z

Closing; obsolete as of #38456.

matticbot · 2019-12-18T10:14:56Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~30 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding       +251 B  (+0.0%)      +30 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

ockham added 2 commits December 16, 2019 13:45

SPT: Move Template Selector files into separate directory

b6207a8

Moar isolating

2b197af

ockham added [Status] In Progress [Goal] Page Templates labels Dec 16, 2019

ockham requested review from a team as code owners December 16, 2019 08:09

ockham self-assigned this Dec 16, 2019

Move more styles

e06b61a

ockham force-pushed the isolate/spt-template-selector branch from 3cbbe53 to e06b61a Compare December 16, 2019 08:13

ockham added 2 commits December 16, 2019 14:23

Fix a path

c2bbed8

Update snapshots

b73066c

ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 16, 2019

sirreal reviewed Dec 16, 2019

View reviewed changes

ockham added 2 commits December 16, 2019 16:16

Use variables from @wordpress/base-styles

f4adda3

Have FSE depend on @wordpress/base-styles

ba960df

ockham requested a review from a team as a code owner December 16, 2019 10:18

Fix import

7b3639e

simison reviewed Dec 16, 2019

View reviewed changes

ockham mentioned this pull request Dec 16, 2019

Base Styles: Import colors into variables WordPress/gutenberg#19159

Merged

6 tasks

ockham added 2 commits December 16, 2019 16:54

Aaand add back other needed import

40b73e8

Add workaround

be80333

simison reviewed Dec 16, 2019

View reviewed changes

Moar workaround

36920fc

ockham closed this Dec 17, 2019

ockham deleted the isolate/spt-template-selector branch December 17, 2019 09:53

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPT: Isolate TemplateSelectorControl component #38445

SPT: Isolate TemplateSelectorControl component #38445

ockham commented Dec 16, 2019 •

edited

Loading

matticbot commented Dec 16, 2019

sirreal left a comment

sirreal Dec 16, 2019

ockham Dec 16, 2019

sirreal Dec 16, 2019

ockham Dec 16, 2019

ockham Dec 16, 2019

simison Dec 16, 2019

ockham Dec 16, 2019

ockham Dec 16, 2019

simison Dec 16, 2019

simison Dec 16, 2019

simison Dec 16, 2019

ockham Dec 16, 2019

simison Dec 16, 2019

simison Dec 16, 2019

ockham Dec 16, 2019

simison Dec 16, 2019

simison commented Dec 16, 2019

simison Dec 16, 2019

ockham Dec 16, 2019

Copons commented Dec 16, 2019

ockham commented Dec 17, 2019

matticbot commented Dec 18, 2019 •

edited

Loading

		@@ -0,0 +1,189 @@
		@import '~@wordpress/base-styles/variables';

	@import '~@wordpress/base-styles/z-index';
	@import '~@wordpress/base-styles/colors';
	@import '~@wordpress/base-styles/variables';
	@import '~@wordpress/base-styles/breakpoints';
	@import '~@wordpress/base-styles/mixins';
	@import '~@wordpress/base-styles/animations';

		$wp-org-sidebar-reduced: 36px;
		$wp-org-sidebar-full: 160px;

SPT: Isolate TemplateSelectorControl component #38445

SPT: Isolate TemplateSelectorControl component #38445

Conversation

ockham commented Dec 16, 2019 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Dec 16, 2019

sirreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison commented Dec 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copons commented Dec 16, 2019

ockham commented Dec 17, 2019

matticbot commented Dec 18, 2019 • edited Loading

ockham commented Dec 16, 2019 •

edited

Loading

matticbot commented Dec 18, 2019 •

edited

Loading